Skip to content

fix(workflows): enforce sandbox sender save validation - #111424

Closed
Silthus wants to merge 15 commits into
PostHog:masterfrom
Silthus:feat/workflows-sandbox-validation
Closed

Silthus wants to merge 15 commits into
PostHog:masterfrom
Silthus:feat/workflows-sandbox-validation

Conversation

@Silthus

@Silthus Silthus commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Merge risk: medium (qa-swarm danger rubric)

  • The check runs in the shared email input validation for every hog function and workflow save.
  • It adds one integration query per save when sender IDs are present.

Problem

Teams can save sandbox senders in broadcasts, destinations and custom sender identities. The sandbox sender supports workflow email steps and test sends.

Implements Silthus/posthog#231, following the sandbox sender spec.

Changes

  • Broadcasts and destinations reject the sandbox sender with an actionable 400.
  • Sandbox email steps reject other senders, custom From addresses, From names, and Reply-To.
  • New selections require the project's flag; unchanged stored sender configurations remain editable after it turns off.
  • Workflow drafts enforce these rules alongside the existing lenient editor validation.
  • Mechanical: request caches share sender lookups and flag evaluation across email steps.

Stored configurations match the complete From object and Reply-To for the same action. Matching only the sender ID would allow edited identity overrides.

This fork PR includes #111368. Review this layer.

Before:

flowchart LR
    Save[Save email configuration] --> Validate[Existing validation] --> Stored[Stored configuration]
    classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff;
    classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff;
    classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000;
    class Save phRed;
    class Validate phBlue;
    class Stored phGray;
Loading

After:

flowchart LR
    Save[Save email configuration] --> Sandbox[Sandbox sender rules]
    Sandbox --> Error[Actionable 400]
    Sandbox --> Validate[Existing validation] --> Stored[Stored configuration]
    classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff;
    classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff;
    classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000;
    classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000;
    class Save phRed;
    class Sandbox,Validate phBlue;
    class Stored phGray;
    class Error phYellow;
Loading

How did you test this code?

The API tests use real Postgres and a facade-created sandbox row. Analytics is mocked at the external SDK boundary.

Verify with .codex/with-flox hogli test products/workflows/backend/tests/api/test_sandbox_sender_validation.py. Allowed workflow saves return 201; rejected selections return actionable 400 responses.

Test rationale: test_literal_off_domain_sender_override_is_rejected_at_save and test_stored_off_domain_sender_override_survives_a_resave cover own-domain overrides. New API cases cover sandbox policy, lenient drafts and stored configurations; the public invocation serializer covers test-send acceptance.

Retained local proof at 97b8663

The recovered logs record outside-in red and green results before and after each fix. Rejected selections initially returned 201; malformed inputs returned 500. The final API suite asserts 400 responses. Unchanged stored sender edits initially returned 400 and now return 200.

API and adjacent input-validation suites: 179 passed in 23.76s
Repo-wide mypy: no issues in 22549 source files
Strict preflight: 0 failures
Ruff lint and formatting: passed
OpenAPI regeneration: no generated diff

The final suite used a fully migrated isolated Postgres database after shared test-database setup failed. No tests were skipped and no database was mocked.

Unchecked: delivery, frontend rendering, and remote patch coverage. These tests do not read event properties; no new-events-schema expansion applies.

Backend CI and Python CI await maintainer approval. This is an unrun CI gap, not a green result.

Release status

  • No feature flag controls this change
  • This change is behind a feature flag and is not available to users
  • This change makes a previously flagged feature available to everyone

Automatic notifications

  • Publish to changelog?

Docs update

No existing document covers sender save validation.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Codex, GPT-6.1 Sol. The inherited base is Claude Code, claude-opus-5-5, documented in #111368.

Skills: /tdd, /writing-tests, /improving-drf-endpoints, /writing-user-facing-copy, /running-ci-preflight, /qa-swarm, /stacking-prs, /writing-pr-descriptions, /writing-voice, /unslop, /principle-prove-it-works, /hogli, /querying-local-postgres, /reviewing-with-coderabbit, /leaving-pr-comments, /writing-for-agents.

CodeRabbit CLI was signed out; the local pass was skipped. The fresh QA swarm and remote CodeRabbit reviewed this layer on 97b86630a25.

One clean round: six GPT-6.1 Sol lenses found no issues. Claude Sonnet5.5's flag-check duplication suggestion was rejected after both matching gates were verified.

CodeRabbit's duplicate-validation nit was rejected: the precheck enforces policy before lenient draft validation can swallow errors.

No fixes or decisions remain open. The code is ready for human review; the PR remains draft with the maintainer-approval CI gap above.

No duplicate validation PR was found. Fixtures use invented example.com addresses. GitHub denied assigning the verified author due account permissions.

Silthus and others added 15 commits October 3, 2026 09:48
…lusions

Unfinished: the red/green proof runs and the OpenAPI regeneration were
interrupted by a devbox rebalance. Tests are written but not yet run green;
generated OpenAPI output is not regenerated. Continue test-first.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ender endpoint

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…equire write scope

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…quest_access comment

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@trunk-io

trunk-io Bot commented Oct 3, 2026

Copy link
Copy Markdown

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

📝 Walkthrough

Walkthrough

The change adds a managed sandbox email sender that the integration API can provision or refresh. It adds settings and integration guards, and rejects sandbox sender use in broadcasts and unsupported workflow email configurations. Workflow validation uses request-scoped caches and retains existing email values to allow unchanged stored configurations. Tests cover provisioning, management restrictions, validation, and integration lifecycle behavior.

Priority: ⬇️ Low

Merge Risk: 🔵 Low · up to 97b86

Sandbox sender validation remains enforced on the described save paths. Clarifying why drafts need the separate check would help prevent a future regression, but the change is mergeable with that follow-up.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 97b86

The change adds meaningful restrictions on sandbox sender use and preserves project-scoped access. No concrete authorization bypass was established. Recovery during sender configuration changes and deployment rollback remains insufficiently demonstrated, so the risk is not minimal.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — Application configuration is project-scoped, but enabled projects use a centrally configured sender address and domain whose SES identity is intentionally shared. A failure of that shared identity could affect multiple enabled projects; effective delivery isolation and the maximum affected population are not established by the inspected evidence.

Trust Boundaries and Controls

  • observed — The new action provisions using the resolved team ID, and existing routing filters and permission checks constrain access to that team. Generic native creation cannot directly overwrite the reserved managed row: it validates an email address, derives its integration ID from that address, and accepts only native provider choices.
  • observed — Normal updates cannot change the workflow's origin product. Status-only activation revalidates stored actions, and staged publication re-enters the normal serializer under a database lock. These are counterevidence to bypassing sender eligibility by changing product ownership or promoting a draft without action validation.

Resilience and Maintainability Implications

  • inferred — If reload publication fails after a refresh saves configuration, retrying with the same configuration skips publication. The refresh operation itself therefore does not repair that interrupted transition. Whether workers independently recover, and whether this can affect sender identity during rotation or rollback, remains unverified rather than an established security finding.

Hardening Proposals

  • proposed — Make managed sender refresh demonstrably convergent after notification failure, and establish an operational owner and recovery procedure for shared SES identity loss, configuration rotation, and rollback. These are resilience proposals, not verified vulnerabilities.
🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The description explains the problem, user-visible changes, testing and test rationale, release status, and agent context. It is detailed and mostly follows the repository template; the missing sessio…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
products/workflows/backend/presentation/views/hog_flow.py (1)

1620-1624: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove the duplicate sandbox validation call.

InputsItemSerializer.validate already calls validate_sandbox_email_sender for email and native_email inputs (posthog/cdp/validation.py Line 828). This loop calls the same validator for the same values. Lenient drafts are the exception: they use is_valid() without raising, so this loop is the only enforcement on that path. If that is the reason for the loop, add a short comment. Without the comment, a later cleanup can remove the only draft-path guard.


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
  • Review profile: QUIET
  • Plan: Enterprise
  • Run ID: 218355ec-146b-4ad4-a8a1-ecf043f2cb45
📥 Commits

Reviewing files that changed from the base of the PR and between 427607e and 97b8663.

⛔ Files ignored due to path filters (1)
  • products/integrations/frontend/generated/api.ts is excluded by !**/generated/**
📒 Files selected for processing (15)
  • posthog/api/integration.py
  • posthog/cdp/test/test_validation.py
  • posthog/cdp/validation.py
  • posthog/models/integration/__init__.py
  • posthog/models/integration/email.py
  • posthog/settings/ses.py
  • products/integrations/mcp/tools.yaml
  • products/workflows/backend/facade/api.py
  • products/workflows/backend/facade/contracts.py
  • products/workflows/backend/presentation/views/hog_flow.py
  • products/workflows/backend/services/sandbox_sender.py
  • products/workflows/backend/test/test_sandbox_sender_lifecycle.py
  • products/workflows/backend/tests/api/test_email_reputation.py
  • products/workflows/backend/tests/api/test_sandbox_sender.py
  • products/workflows/backend/tests/api/test_sandbox_sender_validation.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review.

@scheduled-actions-posthog

Copy link
Copy Markdown
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the stale label – otherwise this will be closed in another week. If you want to permanently keep it open, use the waiting label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant